improves (map) #193
improves (map) #193
Conversation
agzam
force-pushed
the
map-improvements-to-fix-memory-leaks
branch
from
d796186
to
1507d7a
Compare
|
Also, great catch! |
I had no access to a Mac for several months, which forced me to venture into AwesomeWM, which is also Lua-based, and of course I picked it so I could use Fennel. It has a more verbose logging option, and some weird messages with I need to do some diffing and see if I made some other improvements that could also be moved here now. This change incidentally fixed a long-standing personal headache for me. Hammerspoon didn't hang once in the past few days for me. |
Glad it's fixed! Can't say I experienced that but yeah, glad it's resolved. Only behavior I've observed is it eventually crashing on Sonoma. Between the recent hammerspoon release and this fix, that might just fix it. Btw have you started using WezTerm? Switched to it a bit ago after I found out it's also configured with lua and was able to get fennel there. One nuance though is they didn't implement the debug interface so you'll have to stub out the APIs so macros work as expected. I can take on improving the reduce implementation and writing tests for it |
|
WezTerm? No, never heard of it. Aww... yet another rabbit-hole :) I'm using Kitty, as my terminal needs lately have been quite unsophisticated. I usually run things inside Emacs. My most recent epiphany is that mpv is also configurable with Lua, and I've been thinking if I need to do some hacking on that. It's an incredibly liberating approach to watch any video content when you can control the video playback directly from your editor - you can pause/resume, speed it up, control volume, etc. Great for note taking, for example. And of course, with Fennel/Lua, you can do the opposite as well - e.g., jump to things in your editor from the playing video. |
|
I actually have been plagued with Hammerspoon hangs for some time. Strangely only on one of my two machines. I just updated to v1.0.0 the other day, though, and haven't been seen a hang yet. Here's what I'll do:
Also, I use wezterm and I am a big fan as well, though my config is super simple and it's still in Lua. |
| @@ -125,6 +125,10 @@ | |||
| (set ct (+ ct 1)))) | |||
| ct) | |||
|
|
|||
| (fn apply [f ...] | |||
| (let [args [...]] | |||
There was a problem hiding this comment.
do you have to assign the name here or can you just return (f (table.unpack ...))?
There was a problem hiding this comment.
This seems to 'double nest' it. E.g. if I call (apply add [1 2]) in apply args is { { 1, 2} }
There was a problem hiding this comment.
I don't think (f (table.unpack ...)) works.
if I call (apply add [1 2])
You're giving it a single argument. (apply) usually works with multiple, in your case you'd call it (apply add 1 2)
There was a problem hiding this comment.
what's the point of apply, then? Couldn't you just call (add 1 2)?. As I understand it (from clojure), (apply add [1 2])) should be equivalent to (add 1 2)
There was a problem hiding this comment.
Have you given this one more thought?
There was a problem hiding this comment.
And I figure we should probably work in the same way as clojure
There was a problem hiding this comment.
Let's put a pin on this one. It definitely requires some more thought. Even fennel-cljlib implementation is not so trivial.
We need to handle cases like:
user> (apply print [1 2 3])
1 2 3;; => nil
user> (apply print 1 [2 3])
1 2 3;; => nil
user> (apply print [1 2] [3 4])
[1 2] 3 4;; => nil
Why doesn't Clojure unpack both vectors in the last case I have no idea, but that's how it seems to work.
Seven peduncles, I had no idea how Clojure's (apply) is unpredictable, lol:
user> (apply print [:one :two :three])
:one :two :three;; => nil
user> (apply print [:one :two :three] [:four])
[:one :two :three] :four;; => nil
user> (apply print :one :two :three)
java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Keyword
(apply print [:one :two :three] :four)
java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Keyword There was a problem hiding this comment.
oh that's strange. I don't think I have tried to use apply with more than 2 args (a function and a sequence).
I am ok with figuring this out later. We can remove this and merge the rest?
There was a problem hiding this comment.
hmmm... apply is used though. in (map) particularly. I'd say let's merge it as is and then maybe improve it later? I think for general use cases it works. Or do you prefer to get it right instead and not to increase the tech-debt budget?
There was a problem hiding this comment.
Seems that clojure basically only unpack the last argument, which must be a sequence. This implementation collects all args into a table and then unpacks that, which is also incorrect.
map only uses apply with two args, so what I think might be best would be to write this apply for 2 args [fn seq] and unpack that seq, we can later add support for extra intermediate non-splices args between the function and the last sequence.
Our previous implementation of (map) apparently would leave around a GCiable structures that for some reason would hang around, in rare cases causing memory-leaks. This bug, I think, usually manifests itself on multi-monitor setups. What I used to observe: for no apparent reason Hammerspoon hangs, and the system would "swallow" certain keys - e.g., "a", "w", "j" - the keys on the main modal. Which consequently makes it difficult to type something like `"killall Hammerspoon"` in the terminal, and one has to use ActivityMonitor to kill and restart Hammerspoon, the tray icon wouldn't work either. This refactoring also makes (map) similar to Clojure function, you can feed multiple collections (tables) into it.
Suggested-by: Grazfather <[email protected]> https://github.com/agzam/spacehammer/pull/193/files#r1714459770
agzam
force-pushed
the
map-improvements-to-fix-memory-leaks
branch
from
1507d7a
to
4d2512e
Compare
|
I did some rebasing and addressed the comments, give it another gander, folks? @Grazfather @jaidetree |
Thanks Ag! @Grazfather could you please test it? My config is operating off of the spoon branch + tweaks for nix and is very fragile right now |
I only couldn't convert the last clause
Our previous implementation of
mapapparently would leave around GCiable structures that for some reason would hang around, in rare cases causing memory-leaks.This bug, I think, usually manifests itself on multi-monitor setups. What I used to observe: for no apparent reason Hammerspoon hangs, and the system would "swallow" certain keys - e.g., "a", "w", "j" - the keys on the main modal. Which consequently makes it difficult to type something like
"killall Hammerspoon"in the terminal, and one has to use ActivityMonitor (which would show HS eating up like gigs of mem... crazy) to kill and restart Hammerspoon (using only mouse), even the tray icon wouldn't work.This refactoring makes the
(map)similar to Clojure function, you can feed multiple collections (tables) into it.Possibly fixes: #186, #164, #77